Skip to content

test: Add unit tests for ParseNumberFromBrackets - #3878

Open
damansingh1313 wants to merge 2 commits into
apache:mainfrom
damansingh1313:test-parse-number-from-brackets
Open

test: Add unit tests for ParseNumberFromBrackets#3878
damansingh1313 wants to merge 2 commits into
apache:mainfrom
damansingh1313:test-parse-number-from-brackets

Conversation

@damansingh1313

Copy link
Copy Markdown

pyiceberg/utils/parsing.py was the only module under pyiceberg/utils without a dedicated test. ParseNumberFromBrackets backs parsing of bucket[N], truncate[N] and fixed[L], so cover its match behaviour and the ValidationError raised on malformed input.

Rationale for this change

Are these changes tested?

It is a unit tests which are tested in local console.

Are there any user-facing changes?

pyiceberg/utils/parsing.py was the only module under pyiceberg/utils
without a dedicated test. ParseNumberFromBrackets backs parsing of
bucket[N], truncate[N] and fixed[L], so cover its match behaviour and the
ValidationError raised on malformed input.
@damansingh1313

Copy link
Copy Markdown
Author

Hi @kevinjqliu, @Fokko
Could you please review the PR.

Comment thread tests/utils/test_parsing.py Outdated
Comment thread tests/utils/test_parsing.py Outdated
Comment thread tests/utils/test_parsing.py Outdated
Verify the full ValidationError message for the invalid-input cases and
parametrize them. Remove the multi-digit case (already covered) and the
cases that relied on re.search tolerating text around the pattern.

@rambleraptor rambleraptor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great test fix. Thanks for doing this!

@damansingh1313
damansingh1313 requested a review from ebyhr September 2, 2026 05:47

@Fokko Fokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this @damansingh1313 and thanks @rambleraptor and @ebyhr for the review 🚀

@Fokko
Fokko enabled auto-merge September 7, 2026 09:03
@rambleraptor

Copy link
Copy Markdown
Collaborator

@damansingh1313 sometime today, can you try pushing up an empty commit? A couple GitHub runners seem to have stalled out and it's stopping merge

@Fokko @kevinjqliu let me know if this isn't the right method. I'm not sure if this is in the merge queue or not.

@damansingh1313

damansingh1313 commented Sep 8, 2026

Copy link
Copy Markdown
Author

@rambleraptor

Sure, I can give it a try but it will again require approval i guess, it should be fine?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants